Skip to content

Add channel settings tab demo#204

Open
nickmisasi wants to merge 4 commits intomasterfrom
channel-settings-pluggable
Open

Add channel settings tab demo#204
nickmisasi wants to merge 4 commits intomasterfrom
channel-settings-pluggable

Conversation

@nickmisasi
Copy link
Copy Markdown
Contributor

@nickmisasi nickmisasi commented Mar 13, 2026

Summary

  • Register a ChannelSettingsTab pluggable to smoke-test the new channel settings tab plugin API
  • Add a minimal ChannelSettingsSmokeTest component that renders channel info, a dirty-state input, and a tab-switch error banner
  • Make GetTeams() calls non-fatal in ensureDemoUser, ensureDemoChannels, OnActivate, and OnDeactivate so the plugin can start even when the Teams table has NULL string columns (core server bug where CompanyName is NULL)
    Needs Support pluggable channel settings tabs mattermost#35591

Test plan

  • Build and deploy the plugin to a Mattermost instance with channel settings pluggable support
  • Open Channel Settings on a public/private channel and confirm the Smoke Test tab appears
  • Click the tab and verify it renders the channel display name, name, and ID
  • Type into the input to mark the tab dirty, then try switching tabs to confirm the unsaved-change warning
  • Verify the plugin starts cleanly even if GetTeams() returns an error (check server logs for warn-level messages instead of crashes)

🤖 Generated with Claude Code

Register a ChannelSettingsTab pluggable for smoke-testing the new
channel settings tab API. The component renders channel info, a
dirty-state input, and a tab-switch error banner.

Make GetTeams() calls non-fatal in ensureDemoUser, ensureDemoChannels,
OnActivate, and OnDeactivate so the plugin can start even when the
Teams table has NULL string columns (core server bug).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@nickmisasi nickmisasi requested a review from hanzei as a code owner March 13, 2026 14:50
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 13, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 56223aa7-d4fd-4ab3-ba40-3515d255f266

📥 Commits

Reviewing files that changed from the base of the PR and between 2b819ee and 0728245.

📒 Files selected for processing (1)
  • webapp/src/plugin.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • webapp/src/plugin.jsx

📝 Walkthrough

Walkthrough

Server activation/deactivation and configuration now log warnings and continue on team/query or message-post failures instead of returning errors. A new React component, ChannelSettingsSmokeTest, was added and registered as a Channel Settings tab in the plugin.

Changes

Cohort / File(s) Summary
Server hook activation / deactivation
server/activate_hooks.go
OnActivate and OnDeactivate: failures querying teams now log a warning and skip posting messages instead of returning an error; teams without demoChannelID are skipped with a warning; posting failures are logged and processing continues.
Server configuration / demo setup
server/configuration.go
Demo user/channel setup: failures to fetch teams now log warnings and skip related actions instead of returning errors; demo channel setup may return an empty map with nil error to allow graceful skipping.
Frontend smoke test component
webapp/src/components/channel_settings_smoke_test.jsx
Adds default-export ChannelSettingsSmokeTest React component showing basic channel fields, a text input to simulate unsaved changes (calls setAreThereUnsavedChanges), save/reset handlers, optional registerSaveBarHandlers integration, and PropTypes.
Plugin registration
webapp/src/plugin.jsx
Imports ChannelSettingsSmokeTest and conditionally registers it as a Channel Settings tab via registry.registerChannelSettingsTab?.(...) with uiName: 'Demo Plugin', component, and icon.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add channel settings tab demo' clearly and concisely summarizes the main change—registering a new Channel Settings tab component and smoke-test feature.
Description check ✅ Passed The description is well-related to the changeset, detailing the registration of the ChannelSettingsTab pluggable, the new component, and error-handling improvements for GetTeams() calls.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch channel-settings-pluggable

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
webapp/src/components/channel_settings_smoke_test.jsx (1)

17-24: Consider defensive access for channel properties.

While channel is marked as required in PropTypes, accessing nested properties without defensive checks could cause runtime errors if the channel object has an unexpected shape. For a smoke test component this is likely acceptable, but consider using optional chaining for robustness.

♻️ Optional defensive access
             <div style={{marginTop: '12px'}}>
-                <strong>{'Display Name: '}</strong>{channel.display_name}
+                <strong>{'Display Name: '}</strong>{channel?.display_name}
             </div>
             <div style={{marginTop: '4px'}}>
-                <strong>{'Channel Name: '}</strong>{channel.name}
+                <strong>{'Channel Name: '}</strong>{channel?.name}
             </div>
             <div style={{marginTop: '4px'}}>
-                <strong>{'Channel ID: '}</strong>{channel.id}
+                <strong>{'Channel ID: '}</strong>{channel?.id}
             </div>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@webapp/src/components/channel_settings_smoke_test.jsx` around lines 17 - 24,
Accessing channel.display_name, channel.name and channel.id without defensive
checks can throw if channel has an unexpected shape; update the JSX to use
optional chaining and sensible fallbacks (e.g. channel?.display_name ?? '' ,
channel?.name ?? '' , channel?.id ?? '') so missing properties render safely.
Locate the render usage of channel.display_name / channel.name / channel.id in
the channel settings smoke test component and replace direct property access
with the optional-chaining + fallback pattern to make the component robust.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@webapp/src/components/channel_settings_smoke_test.jsx`:
- Around line 17-24: Accessing channel.display_name, channel.name and channel.id
without defensive checks can throw if channel has an unexpected shape; update
the JSX to use optional chaining and sensible fallbacks (e.g.
channel?.display_name ?? '' , channel?.name ?? '' , channel?.id ?? '') so
missing properties render safely. Locate the render usage of
channel.display_name / channel.name / channel.id in the channel settings smoke
test component and replace direct property access with the optional-chaining +
fallback pattern to make the component robust.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6ff8b33f-4acf-4e51-a81e-8586416deed2

📥 Commits

Reviewing files that changed from the base of the PR and between 9c80775 and b82d856.

📒 Files selected for processing (4)
  • server/activate_hooks.go
  • server/configuration.go
  • webapp/src/components/channel_settings_smoke_test.jsx
  • webapp/src/plugin.jsx

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@nickmisasi nickmisasi changed the title Add channel settings tab smoke test Add channel settings tab demo Mar 13, 2026
Copy link
Copy Markdown
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I agree with the non-fatal changes

msg := fmt.Sprintf("OnActivate: %s", manifest.Id)
if err := p.postPluginMessage(team.Id, msg); err != nil {
return errors.Wrap(err, "failed to post OnActivate message")
p.API.LogWarn("Failed to query teams OnActivate, skipping activation messages", "error", err.Error())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's strange that GetTeams fails. I wonder if we should still fail loudly as that makes things easier for QA to debug.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to see a typescript file. not a blocker

@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Mar 19, 2026
- Register save/reset via registerSaveBarHandlers; remove inline tab-switch banner
- Set icon to fa fa-plug to match other demo plugin surfaces (MainMenu, channel header, etc.)

Made-with: Cursor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2: Dev Review Requires review by a core committer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants